feat(run-engine,run-store): completed-waitpoint envelope, read-time resolver, and the fail-loud coverage check - #4779

internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts (3)

156-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

This assertion cannot fail; express it against the crash count.

Line 146 already asserts pgRun.attemptNumber is exactly 1, so 1 - 1 <= 1 always holds. The comment says the line proves the per-crash bound, but it proves nothing. The last test in this file states the same property correctly against faults.fired(...).

♻️ Proposed change

-        // The bound: one crash costs at most one attempt number.
-        expect(pgRun.attemptNumber! - 1).toBeLessThanOrEqual(1);
+        // The bound: pgAttempt - maxLoggedAttempt <= crashCount.
+        expect((pgRun.attemptNumber ?? 0) - 1).toBeLessThanOrEqual(
+          faults.fired("afterPgBeforeRedis")
+        );

---

383-384: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The test name claims two crashes, but the assertion accepts one.

expect(crashes).toBeGreaterThanOrEqual(1) passes when only one fault fires. The two-crash scenario named in the title and in the comment on Line 390 is then never exercised, and the bound assertion on Line 391 degrades to the single-crash case. This is the same silent-pass failure mode the fired() guards elsewhere in this file exist to prevent.

Assert the exact expected count.

♻️ Proposed change

         const crashes = faults.fired("afterPgBeforeRedis");
-        expect(crashes).toBeGreaterThanOrEqual(1);
+        expect(crashes).toBe(2);

---

197-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Three dequeue sites index dequeued[0]! without the length assertion this file uses elsewhere. The first and third tests assert expect(dequeued.length).toBe(1) before indexing. The other three do not, so an empty queue produces a TypeError instead of the intended assertion failure.

• internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts#L197-L204: add expect(dequeued.length).toBe(1) after the dequeue call in the afterRedisBirthBeforePg test.
• internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts#L315-L320: add the same assertion after the dequeue call in the stale-snapshot test.
• internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts#L364-L371: add the same assertion after the dequeue call in the two-crash test.

internal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.ts (1)

232-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

This test cannot fail if the birth omits the completion TTL.

The comment states the invariant: a born-terminal run never transitions again, so the birth itself must apply the completion TTL. The assertions only check that both keyspaces are readable. They pass whether or not any expiry was set, and the non-terminal run exists only as an unused comparison.

Assert the expiry directly with a raw Redis client, as taskRunExecutionSnapshotStore.waitpointCycles.test.ts does for cycle keys: expect a positive pttl on the terminal run's key and -1 on the non-terminal run's key.

internal-packages/run-store/src/redisSnapshotStore.ts (1)

459-494: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the shared row-decode and head-resolution block.

getSince and getSinceCreatedAt now carry the same loop, the same headSurvived tracking, the same rows.reverse(), and the same head attribution. The offsets (i = 3, stride 4) and the reply layout must stay identical in both, so a future change to the Lua reply shape must be applied twice. Extract one private helper that takes the reply and returns { entries, headWaitpointIds }.

♻️ Sketch

`#decodeSinceReply`(
  reply: string[],
  environmentId: string | undefined,
  runId: string
): { entries: SnapshotRead[]; headWaitpointIds: WaitpointIds } {
  const headOrder = reply[1] ?? "";
  const headDistinct = reply[2] ?? "";
  const rows: SnapshotRead[] = [];
  let headSurvived = false;
  for (let i = 3; i + 3 < reply.length; i += 4) {
    const decoded = this.#decode(
      [reply[i], reply[i + 1], reply[i + 2], reply[i + 3], ""],
      environmentId,
      runId,
      false
    );
    if (decoded) {
      rows.push(decoded);
      if (i === 3) headSurvived = true;
    }
  }
  rows.reverse();
  const head = headSurvived ? rows[rows.length - 1] : undefined;
  const headWaitpointIds = decodeWaitpointIds(
    head !== undefined,
    head ? headOrder : "",
    head ? headDistinct : ""
  );
  if (head) {
    head.completedWaitpointIds = headWaitpointIds;
    if (head.cycle) {
      this.#checkCycleMismatch(runId, head.cycle.count, headWaitpointIds.order.length);
    }
  }
  return { entries: rows, headWaitpointIds };
}

Also applies to: 503-557

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.

internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts (3)

156-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

This assertion cannot fail; express it against the crash count.

Line 146 already asserts pgRun.attemptNumber is exactly 1, so 1 - 1 <= 1 always holds. The comment says the line proves the per-crash bound, but it proves nothing. The last test in this file states the same property correctly against faults.fired(...).

♻️ Proposed change

-        // The bound: one crash costs at most one attempt number.
-        expect(pgRun.attemptNumber! - 1).toBeLessThanOrEqual(1);
+        // The bound: pgAttempt - maxLoggedAttempt <= crashCount.
+        expect((pgRun.attemptNumber ?? 0) - 1).toBeLessThanOrEqual(
+          faults.fired("afterPgBeforeRedis")
+        );

---

383-384: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The test name claims two crashes, but the assertion accepts one.

expect(crashes).toBeGreaterThanOrEqual(1) passes when only one fault fires. The two-crash scenario named in the title and in the comment on Line 390 is then never exercised, and the bound assertion on Line 391 degrades to the single-crash case. This is the same silent-pass failure mode the fired() guards elsewhere in this file exist to prevent.

Assert the exact expected count.

♻️ Proposed change

         const crashes = faults.fired("afterPgBeforeRedis");
-        expect(crashes).toBeGreaterThanOrEqual(1);
+        expect(crashes).toBe(2);

---

197-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Three dequeue sites index dequeued[0]! without the length assertion this file uses elsewhere. The first and third tests assert expect(dequeued.length).toBe(1) before indexing. The other three do not, so an empty queue produces a TypeError instead of the intended assertion failure.

• internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts#L197-L204: add expect(dequeued.length).toBe(1) after the dequeue call in the afterRedisBirthBeforePg test.
• internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts#L315-L320: add the same assertion after the dequeue call in the stale-snapshot test.
• internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts#L364-L371: add the same assertion after the dequeue call in the two-crash test.

internal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.ts (1)

232-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

This test cannot fail if the birth omits the completion TTL.

The comment states the invariant: a born-terminal run never transitions again, so the birth itself must apply the completion TTL. The assertions only check that both keyspaces are readable. They pass whether or not any expiry was set, and the non-terminal run exists only as an unused comparison.

Assert the expiry directly with a raw Redis client, as taskRunExecutionSnapshotStore.waitpointCycles.test.ts does for cycle keys: expect a positive pttl on the terminal run's key and -1 on the non-terminal run's key.

internal-packages/run-store/src/redisSnapshotStore.ts (1)

459-494: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the shared row-decode and head-resolution block.

getSince and getSinceCreatedAt now carry the same loop, the same headSurvived tracking, the same rows.reverse(), and the same head attribution. The offsets (i = 3, stride 4) and the reply layout must stay identical in both, so a future change to the Lua reply shape must be applied twice. Extract one private helper that takes the reply and returns { entries, headWaitpointIds }.

♻️ Sketch

`#decodeSinceReply`(
  reply: string[],
  environmentId: string | undefined,
  runId: string
): { entries: SnapshotRead[]; headWaitpointIds: WaitpointIds } {
  const headOrder = reply[1] ?? "";
  const headDistinct = reply[2] ?? "";
  const rows: SnapshotRead[] = [];
  let headSurvived = false;
  for (let i = 3; i + 3 < reply.length; i += 4) {
    const decoded = this.#decode(
      [reply[i], reply[i + 1], reply[i + 2], reply[i + 3], ""],
      environmentId,
      runId,
      false
    );
    if (decoded) {
      rows.push(decoded);
      if (i === 3) headSurvived = true;
    }
  }
  rows.reverse();
  const head = headSurvived ? rows[rows.length - 1] : undefined;
  const headWaitpointIds = decodeWaitpointIds(
    head !== undefined,
    head ? headOrder : "",
    head ? headDistinct : ""
  );
  if (head) {
    head.completedWaitpointIds = headWaitpointIds;
    if (head.cycle) {
      this.#checkCycleMismatch(runId, head.cycle.count, headWaitpointIds.order.length);
    }
  }
  return { entries: rows, headWaitpointIds };
}

Also applies to: 503-557

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.

internal-packages/run-engine/src/engine/systems/waitpointSystem.ts (3)

490-497: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Add OTEL instrumentation for the completed-waitpoint propagation flow.

The new RunEngine flow reads completion envelopes and creates a snapshot without tracer or meter instrumentation. Add a trace span and bounded outcome metrics. Use only bounded attributes such as coordinator arm, snapshot status, and success or failure. Do not add run IDs, waitpoint IDs, or record counts as metric attributes.

• internal-packages/run-engine/src/engine/systems/waitpointSystem.ts#L490-L497: instrument envelope collection and record-build outcomes.
• internal-packages/run-engine/src/engine/systems/enqueueSystem.ts#L96-L117: instrument the completed-waitpoint snapshot handoff.

As per coding guidelines, “Integrate OpenTelemetry tracer and meter instrumentation in RunEngine systems for observability.”

📍 Affects 2 files

• internal-packages/run-engine/src/engine/systems/waitpointSystem.ts#L490-L497 (this comment)
• internal-packages/run-engine/src/engine/systems/enqueueSystem.ts#L96-L117

Source: Coding guidelines

---

744-773: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Add approved crumbs to the new snapshot propagation flow.

The new completion-envelope, record-building, and Redis-entry paths contain no crumb markers. Add // @Crumbs markers or `#region `@crumbs blocks with an approved namespace. Request a namespace before adding one because the provided guideline table is unavailable.

• internal-packages/run-engine/src/engine/systems/waitpointSystem.ts#L744-L773: mark envelope classification and coordinator reads.
• internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.ts#L12-L60: mark record and output-variant selection.
• internal-packages/run-store/src/snapshotEntry.ts#L135-L159: mark execution-snapshot entry construction.

As per coding guidelines, “Add crumbs as you write code” and “Do not invent new namespaces.”

📍 Affects 3 files

• internal-packages/run-engine/src/engine/systems/waitpointSystem.ts#L744-L773 (this comment)
• internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.ts#L12-L60
• internal-packages/run-store/src/snapshotEntry.ts#L135-L159

Source: Coding guidelines

---

767-772: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

🔎 Supported by static analysis

🏁 Script executed:

printf '%s\n' '--- applicable repository conventions ---'
find /tmp/coderabbit-repo-knowledge/triggerdotdev-trigger-dev-0bdd0019 -type f -name '*.md' -print
printf '%s\n' '--- waitpointSystem structure and target ---'
ast-grep outline internal-packages/run-engine/src/engine/systems/waitpointSystem.ts
sed -n '1,90p;700,785p' internal-packages/run-engine/src/engine/systems/waitpointSystem.ts
printf '%s\n' '--- coordinator definitions and relevant methods ---'
rg -n --glob '*.{ts,tsx}' 'class (LegacyPostgresWaitpointCoordinator|StoreCoordinator)|readCompletionEnvelopes|LegacyPostgresWaitpointCoordinator' internal-packages

Repository: triggerdotdev/trigger.dev

Length of output: 15028

---

🏁 Script executed:

printf '%s\n' '--- applicable conventions and learnings ---'
cat /tmp/coderabbit-repo-knowledge/triggerdotdev-trigger-dev-0bdd0019/conventions/internal-packages-run-engine-src-engine-systems.md
cat /tmp/coderabbit-repo-knowledge/triggerdotdev-trigger-dev-0bdd0019/learnings/internal-packages-run-engine-src-engine-systems.md
printf '%s\n' '--- coordinator contract and implementations ---'
cat -n internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts
sed -n '1,145p;490,585p' internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts
sed -n '1,90p;495,575p' internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.ts
printf '%s\n' '--- callers of the private read path and waitpoint ID parsing ---'
rg -n -A18 -B18 '`#completedWaitpointRecordsFor`|completedWaitpointRecordsFor|parseWaitpointId|b32hexW' internal-packages/run-engine/src/engine/systems/waitpointSystem.ts internal-packages/run-engine/src/engine/waitpointCoordinator internal-packages packages

Repository: triggerdotdev/trigger.dev

Length of output: 50381

---

🏁 Script executed:

printf '%s\n' '--- completion record builder and resume consumption ---'
cat -n internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.ts
sed -n '450,620p' internal-packages/run-engine/src/engine/systems/waitpointSystem.ts
printf '%s\n' '--- StoreWaitpointCoordinator construction and usage ---'
rg -n -A12 -B8 'new Store|StoreWaitpoint|StoreCoordinator|storeCoordinator|WaitpointCoordinator' internal-packages/run-engine/src/engine --glob '*.ts'
printf '%s\n' '--- exact legacy envelope body ---'
sed -n '90,165p' internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts

Repository: triggerdotdev/trigger.dev

Length of output: 38971

---

🏁 Script executed:

printf '%s\n' '--- all uses of completedWaitpointRecords in the resume path ---'
rg -n -A12 -B12 'completedWaitpointRecords' internal-packages/run-engine/src/engine/systems/waitpointSystem.ts
printf '%s\n' '--- store record read contract and coordinator capabilities ---'
rg -n -A20 -B10 'readCompletionEnvelopes|readRunBlockState|complete\\(|implements WaitpointCoordinator|export class WaitpointStoreCoordinator' internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.ts internal-packages/run-engine/src/engine/waitpointCoordinator/*.test.ts
printf '%s\n' '--- resources relevant to Redis/store routing ---'
rg -n -A8 -B8 'redisOptions|waitpoint.*Store|WaitpointStoreCoordinator|SystemResources' internal-packages/run-engine/src/engine/systems internal-packages/run-engine/src/engine --glob '*.ts'

Repository: triggerdotdev/trigger.dev

Length of output: 50382

---

🏁 Script executed:

sed -n '560,710p' internal-packages/run-engine/src/engine/systems/waitpointSystem.ts
printf '%s\n' '--- targeted record propagation ---'
rg -n -A10 -B10 'completedWaitpointRecords' internal-packages/run-engine/src/engine/systems/waitpointSystem.ts
printf '%s\n' '--- store coordinator interface declaration ---'
sed -n '145,205p' internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.ts
rg -n 'readCompletionEnvelopes' internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.ts

Repository: triggerdotdev/trigger.dev

Length of output: 12481

---

🏁 Script executed:

printf '%s\n' '--- completed waitpoint record contract and resolver ---'
rg -l 'CompletedWaitpointRecord|completedWaitpointRecords' internal-packages packages --glob '*.{ts,tsx}' | head -80
printf '%s\n' '--- relevant declarations and consumers ---'
rg -n -A18 -B12 'completedWaitpointRecords|CompletedWaitpointRecord' internal-packages/run-store packages internal-packages/run-engine/src --glob '*.{ts,tsx}' --glob '!**/*.test.ts' --glob '!**/*.bench.test.ts' | head -500

Repository: triggerdotdev/trigger.dev

Length of output: 50381

---

🏁 Script executed:

sed -n '1,130p' internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.ts
sed -n '584,670p' internal-packages/run-store/src/taskRunExecutionSnapshotStore.ts
rg -n -A18 -B12 'resolveCompletedWaitpoints|CompletedWaitpointResolver|completedWaitpointRecords' internal-packages/run-store/src internal-packages/run-engine/src --glob '!**/*.test.ts' --glob '!**/*.bench.test.ts'

Repository: triggerdotdev/trigger.dev

Length of output: 50381

---

Read store-format envelopes with WaitpointStoreCoordinator.

When a completed waitpoint has format b32hexW, waitpointSystem.ts:767 uses the fixed LegacyPostgresWaitpointCoordinator. A Redis-only waitpoint is omitted, and the resolver can throw UnresolvableWaitpointId with reason no-source. Keep legacy operations on Postgres, but route this envelope read to WaitpointStoreCoordinator. Add an integration test for a Redis-only b32hexW waitpoint.

internal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.ts (1)

273-294: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

#!/bin/bash
# Description: Resolve the effective tsconfig strictness for the run-engine package.
set -euo pipefail

fd -t f 'tsconfig*.json' internal-packages/run-engine --exec cat {} \;
fd -t f 'tsconfig*.json' --max-depth 1 --exec cat {} \;

Repository: triggerdotdev/trigger.dev

Length of output: 391

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- target test excerpt ---'
sed -n '250,305p' internal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.ts

printf '%s\n' '--- package tsconfig files ---'
fd -t f 'tsconfig*.json' internal-packages/run-engine -x sh -c 'echo "--- $1"; cat "$1"' sh {}

Repository: triggerdotdev/trigger.dev

Length of output: 2395

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

for file in \
  internal-packages/run-engine/tsconfig.src.json \
  internal-packages/run-engine/tsconfig.test.json \
  tsconfig.json
do
  if [ -f "$file" ]; then
    echo "--- $file"
    cat "$file"
  else
    echo "--- $file (missing)"
  fi
done

echo '--- tracked TypeScript configuration paths near run-engine ---'
git ls-files '*tsconfig*.json' | grep -E '(^|/)run-engine/|(^|/)tsconfig\.json$' || true

Repository: triggerdotdev/trigger.dev

Length of output: 3848

---

🌐 Web query:

TypeScript TS2454 variable used before being assigned assignment inside try finally definite assignment

💡 Result:

The TypeScript error TS2454, "Variable '...' is used before being assigned," occurs within try-finally blocks because TypeScript’s control flow analysis (CFA) must account for the possibility that code inside the try block could throw an exception at any point [1][2][3]. Consequently, the compiler cannot guarantee that an assignment statement inside a try block successfully executed before reaching the finally block or subsequent code [1][4][5]. ### Why This Happens When you declare a variable (e.g., let x: string;) without an initial value, TypeScript treats it as unassigned [3][5]. Even if you assign a value inside a try block, the compiler assumes that an error could occur before that assignment is reached [1][6]. Because the finally block or subsequent code might execute regardless of whether the try block completed, TypeScript flags the potential usage of an uninitialized variable as an error to prevent runtime undefined issues [3][5]. ### Recommended Solutions 1. Initialize at Declaration (Preferred) The most reliable way to resolve this is to provide an initial value at the point of declaration [3][7]. typescript // Fix: Provide a default value let x: string = ''; try { x = fetchData(); } finally { console.log(x); // Now guaranteed to be assigned } 2. Use a Union Type with undefined If a value is not immediately available, you can explicitly define the variable as potentially undefined [1][6]. This informs the compiler that the variable might not have a value, forcing you to handle the undefined case safely [1][6]. typescript let x: string | undefined; try { x = fetchData(); } finally { if (x !== undefined) { console.log(x); } } 3. Definite Assignment Assertion If you are certain that the variable will be assigned before it is used—and you want to bypass the compiler check—you can use the definite assignment assertion operator (!) [3][5]. Use this sparingly, as it tells the compiler to trust you, potentially hiding actual bugs [7]. typescript let x!: string; try { x = fetchData(); } finally { console.log(x); // Compiler accepts this } In summary, TS2454 is a safeguard against unpredictable control flow [3]. By initializing variables or explicitly handling undefined states, you align your code with TypeScript's safety requirements [3][7].

Citations:

• 1: microsoft/TypeScript#40290
• 2: microsoft/TypeScript#32836
• 3: https://deverrors.com/errors/ts-variable-used-before-assigned
• 4: https://stackoverflow.com/questions/70526138/typescript-error-variable-reswithstatus-is-used-before-being-assigned-ts245
• 5: https://akousa.net/error-codes/ts-2454
• 6: microsoft/TypeScript#15499
• 7: https://deadends.dev/typescript/ts2454-variable-used-before-assigned/

---

Initialize runId before the try block.

The test project enables "strict": true. TypeScript can report TS2454 because runId is assigned inside try and read after finally. Initialize it or restructure the code so the assignment is definite.

internal-packages/run-store/src/delegatingRunStore.forwarding.test.ts (1)

11-17: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add a crumb marker for this new source-level test.

Add // @Crumbs or a `#region `@crumbs block near the source parsing setup. This file currently has no required crumb instrumentation.

As per coding guidelines, “Add crumbs as you write code — not just when debugging.”

Source: Coding guidelines

internal-packages/run-store/src/PostgresRunStore.ts (1)

2033-2059: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

#!/bin/bash
# Description: List TaskRunExecutionSnapshot scalar columns and compare with the echoed literal.
set -euo pipefail

fd -t f 'schema.prisma' --exec rg -n -A 60 'model TaskRunExecutionSnapshot\b' {} \;

fd -t f 'snapshotEntry.ts' internal-packages/run-store/src --exec cat -n

Repository: triggerdotdev/trigger.dev

Length of output: 11760

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- PostgresRunStore context ---'
sed -n '1970,2085p' internal-packages/run-store/src/PostgresRunStore.ts

printf '%s\n' '--- SnapshotEntryInput and related hydrator definitions ---'
rg -n -C 8 'type SnapshotEntryInput|interface SnapshotEntryInput|SnapshotEntryInput|lastHeartbeatAt|entryFromCreateExecutionSnapshot' internal-packages/run-store/src

Repository: triggerdotdev/trigger.dev

Length of output: 50381

---

Add lastHeartbeatAt: null to the Redis-only snapshot echo.

TaskRunExecutionSnapshot declares this nullable column, and the Redis hydrator returns null for it. The echo omits it and bypasses type checking, so callers receive undefined instead of null.

internal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.ts (1)

20-24: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Remove the mocked RunStore.

The empty-object cast is a mocked RunStore. Extract the cohort predicate into a pure function and test that function, or construct the store with a testcontainer-backed RunStore.

As per coding guidelines, “We use vitest exclusively. Never mock anything - use testcontainers instead.”

Source: Coding guidelines

internal-packages/run-store/src/taskRunExecutionSnapshotStore.ts (1)

746-754: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win

One extra Redis round trip per non-head row in the window read.

getSinceCreatedAt attaches completedWaitpointIds to the head row only. Every other row therefore falls into the await this.redis.getSnapshotWaitpointIds(runId, read.id) branch in #hydrate, so a window of take rows costs up to take - 1 extra Redis calls where Postgres served the same window with one query. The comment on Line 749 states that each row carries its own order, which the store does not currently provide.

Two options: return per-row order from the Lua script, or accept an empty order for non-head rows and say so in the comment. If the engine only reads completedWaitpointOrder from the head row, the second option removes the fan-out entirely.

Also applies to: 820-826